Committed improvements #35

AlexWayfer · 2018-02-02T14:52:14Z

Instead of #31, splitted to separate commits.

Sorry, too hard to make separate PRs: one change rely on another.

I can answer ant questions.

There is Add what you changed to CHANGES.md in the Contributing section of README, but there is no section for this.

https://github.com/guard/guard-compat#migrating-your-api-calls

Puma can be managed not only via URL with token, but also via CLI without token.

`:restart_timeout` waits this number of seconds before the next restarting the Puma server (default `1`).

jc00ke · 2018-02-02T17:06:46Z

Separate commits is fine, thank you! I'll try to review this weekend. I really appreciate the work you put into this project.

AlexWayfer · 2018-02-02T18:45:19Z

Separate commits is fine, thank you! I'll try to review this weekend.

Thank you!

I really appreciate the work you put into this project.

I've switched our commercial project with Puma to Guard, and I've found some troubles with server management via guard-puma. So, I want to fix them, improve this library, and make our team and anybody else happy :)

jc00ke · 2018-02-04T19:02:13Z

CHANGES.md

@@ -2,6 +2,7 @@

 ## master

+* Don't notify about start when no start


jc00ke · 2018-02-04T19:04:14Z

lib/guard/puma/runner.rb

-          '--environment' => options[:environment]
-        }
+      puma_options = {
+        (pumactl ? '--config-file' : '--config') => options[:config],


I'm not a fan of this syntax. I see what it's doing, but I'd prefer something more straightforward.

I made private constant with keys and private method for getting right key. What do you think?

jc00ke · 2018-02-04T19:04:37Z

lib/guard/puma/runner.rb

+      puma_options = {
+        (pumactl ? '--config-file' : '--config') => options[:config],
+        '--control-token' => @control_token,
+        (pumactl ? '--control-url' : '--control') => "tcp://#{@control_url}"


Same here. This is a bit too clever for my tastes 😉

Also changed.

jc00ke · 2018-02-04T19:06:42Z

lib/guard/puma/runner.rb

      end
-      [:bind, :threads].each do |opt|
-        puma_options["--#{opt}"] = options[opt] if options[opt]
+      %i[bind threads environment].each do |opt|


Hmm, is there another way these could be checked with out using multiple next's? I find this kind of imperative looping with conditionals to be difficult to reason about.

One next I took out into Array#select, but this is double iterating. 🙁

The second next I deleted with else adding.

jc00ke · 2018-02-04T19:09:51Z

lib/guard/puma/runner.rb

    end

    def halt
-      Net::HTTP.get build_uri('halt')
+      if pumactl


I feel like these can be pushed down a level too so we don't have so many of these conditionals.

Yes, thank you.

jc00ke · 2018-02-05T04:32:20Z

lib/guard/puma.rb

@@ -38,6 +40,8 @@ def start
    end

    def reload
+      return if Time.now - @last_restarted < options[:restart_timeout]


Can you add parens to make the order of operations more clear?

jc00ke · 2018-02-05T04:33:00Z

spec/lib/guard/puma_spec.rb

@@ -142,9 +149,13 @@
    end

    context "with custom :notifications option" do
-      let(:options) { { notifications: [:restarted] } }
+      let(:options) do


I'm a fan of the Semantic Rule so let's change to {} instead of do/end.

OK, no problem.

jc00ke · 2018-02-05T04:35:37Z

spec/lib/guard/puma/runner_spec.rb

+      context "without pumactl" do
+        let(:options) { { pumactl: false } }
+
+        let(:uri) do


I'm a proponent of the Semantic Rule for {} vs do/end so for the let's I prefer to use {}.

Heh, I didn't mean to make this comment twice. I think it came from having 2 browser tabs open for reviews of different commits. Oops!

There were many places with let(:foo) do; end, I replaced all of them.

jc00ke · 2018-04-09T16:27:12Z

Thanks @AlexWayfer I'll take a look at this tonight (hopefully)

Meant to review it over the weekend but life got in the way.

AlexWayfer · 2018-04-09T16:29:32Z

Thank you, no problem. Life is important. We can use a fork for a while.

jc00ke

One minor comment. Thanks again for all of your contributions!

jc00ke · 2018-04-10T16:42:44Z

lib/guard/puma/runner.rb

+      true => {
+        config:      '--config-file',
+        control_url: '--control-url'
+      }.freeze,


These inner hashes don't also need to be frozen. The outer hash being frozen freezes them. ❄️

No: https://repl.it/repls/EsteemedSubstantialPoints

Even Strings in frozen Hashes not frozen, as you can see in this example. So, we maybe should freeze '--config-file' and other strings.

Weird, I must have mistyped something in a previous IRB session.

I don't think it's really necessay to deep freeze the whole thing anyway, at least not right now (given no real issues on it)

Committed improvements

AlexWayfer · 2018-04-10T17:41:36Z

Hooray! Thank you! 🎉

I'll come back with new changes. 😁

AlexWayfer added 9 commits February 2, 2018 16:43

Add master section into CHANGES.md for unreleased changes

c42be83

There is Add what you changed to CHANGES.md in the Contributing section of README, but there is no section for this.

Update versions of dependencies, remove unused pry dependency

13192e2

Slightly improve syntax

9f5158d

Improve guard-compat using

a1b7729

https://github.com/guard/guard-compat#migrating-your-api-calls

Enable colored output for RSpec

651e3b3

Don't notify about Puma start when no start

533223b

Don't stop Puma if it was not started by Guard

dd6945b

Add :pumactl option

67520e3

Puma can be managed not only via URL with token, but also via CLI without token.

Add :restart_timeout option

539b8fc

`:restart_timeout` waits this number of seconds before the next restarting the Puma server (default `1`).

This was referenced Feb 2, 2018

Add master section into CHANGES.md for unreleased changes #32

Closed

Slightly improve syntax #34

Closed

Update versions of dependencies, remove unused pry dependency #33

Closed

jc00ke reviewed Feb 4, 2018

View reviewed changes

CHANGES.md Outdated

@@ -2,6 +2,7 @@

## master

* Don't notify about start when no start

Copy link

Owner

jc00ke Feb 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

jc00ke requested changes Feb 5, 2018

View reviewed changes

AlexWayfer added 3 commits April 4, 2018 18:39

Small syntax improvements

6973c6f

Use system/uri wrapper (depends on pumactl option) for halt command

46637ce

Use Semantic Rule for Block Delimiters

f662b8f

jc00ke requested changes Apr 10, 2018

View reviewed changes

jc00ke approved these changes Apr 10, 2018

View reviewed changes

jc00ke merged commit f662b8f into jc00ke:master Apr 10, 2018

jc00ke added a commit that referenced this pull request Apr 10, 2018

Merge pull request #35 from AlexWayfer/committed_improvements

abdba16

Committed improvements

AlexWayfer deleted the committed_improvements branch April 10, 2018 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Committed improvements #35

Committed improvements #35

AlexWayfer commented Feb 2, 2018

jc00ke commented Feb 2, 2018

AlexWayfer commented Feb 2, 2018

jc00ke Feb 4, 2018

jc00ke Feb 4, 2018

AlexWayfer Apr 4, 2018

jc00ke Feb 4, 2018

AlexWayfer Apr 4, 2018

jc00ke Feb 4, 2018

AlexWayfer Apr 4, 2018

jc00ke Feb 4, 2018

AlexWayfer Apr 4, 2018

jc00ke Feb 5, 2018

AlexWayfer Apr 4, 2018

jc00ke Feb 5, 2018

AlexWayfer Apr 4, 2018

jc00ke Feb 5, 2018

jc00ke Feb 5, 2018

AlexWayfer Apr 4, 2018

jc00ke commented Apr 9, 2018

AlexWayfer commented Apr 9, 2018

jc00ke left a comment

jc00ke Apr 10, 2018

AlexWayfer Apr 10, 2018

AlexWayfer Apr 10, 2018

jc00ke Apr 10, 2018

AlexWayfer commented Apr 10, 2018

		@@ -2,6 +2,7 @@

		## master

		* Don't notify about start when no start

Committed improvements #35

Committed improvements #35

Conversation

AlexWayfer commented Feb 2, 2018

jc00ke commented Feb 2, 2018

AlexWayfer commented Feb 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jc00ke commented Apr 9, 2018

AlexWayfer commented Apr 9, 2018

jc00ke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexWayfer commented Apr 10, 2018